make labels.NewRequirement returns aggregated field.ErrorList #97538

lingsamuel · 2020-12-28T04:44:49Z

Make labels.NewRequirement returns aggregated field.ErrorList, make nodeaffinity parsing functions use it

Signed-off-by: Ling Samuel lingsamuelgrace@gmail.com

What type of PR is this?

/kind feature

What this PR does / why we need it:

See #96167

Which issue(s) this PR fixes:

Fixes #96167

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2020-12-28T04:44:57Z

Hi @lingsamuel. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lingsamuel · 2020-12-28T05:05:31Z

/cc @alculquicondor

yuxiaobo96 · 2020-12-28T06:27:37Z

/ok-to-test

staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity_test.go

staging/src/k8s.io/apimachinery/pkg/util/validation/field/path.go

staging/src/k8s.io/apimachinery/pkg/labels/selector.go

pkg/scheduler/apis/config/validation/validation_pluginargs_test.go

staging/src/k8s.io/apimachinery/pkg/labels/selector.go

staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity.go

alculquicondor

/approve
for scheduling

Please squash

…odeaffinity parsing function use it Signed-off-by: Ling Samuel <lingsamuelgrace@gmail.com>

lingsamuel · 2021-01-11T08:27:30Z

Commits rebased.

/cc @lavalamp

lingsamuel · 2021-01-11T08:53:07Z

/retest

lavalamp · 2021-01-11T22:14:30Z

staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity_test.go

-					Field:  "[2].matchExpressions[0]",
-					Detail: `invalid label key "invalid key": name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName',  or 'my.name',  or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')`,
+					Field:  "[2].matchExpressions[0].key",
+					Detail: `name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName',  or 'my.name',  or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')`,


"name part" what other part of the key is there?

The "name part" generated by IsQualifiedName, here is the whole key (.key indicates it).

kubernetes/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go

Lines 67 to 69 in 6539513

if !qualifiedNameRegexp.MatchString(name) {

errs = append(errs, "name part "+RegexError(qualifiedNameErrMsg, qualifiedNameFmt, "MyName", "my.name", "123-abc"))

}

kubernetes/staging/src/k8s.io/apimachinery/pkg/labels/selector.go

Lines 874 to 876 in a1f8dc4

func validateLabelKey(k string, path *field.Path) *field.Error {

if errs := validation.IsQualifiedName(k); len(errs) != 0 {

return field.Invalid(path, k, strings.Join(errs, "; "))

Will a user understand this from the message?

can we just remove "name part"?

lavalamp · 2021-01-11T22:15:35Z

staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity_test.go

@@ -218,8 +218,8 @@ func TestPreferredSchedulingTermsScore(t *testing.T) {
 				},
 				&field.Error{
 					Type:   field.ErrorTypeInvalid,
-					Field:  "[2].matchExpressions[0]",
-					Detail: `invalid label key "invalid key": name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName',  or 'my.name',  or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')`,
+					Field:  "[2].matchExpressions[0].key",


If there are multiple keys (multiple expressions), how does the user know which one this message is about?

Output contains field. It would like [[2].matchExpressions[0].key: Invalid value: REASON, [2].matchExpressions[1].key: Invalid value: REASON]

kubernetes/staging/src/k8s.io/apimachinery/pkg/util/validation/field/errors.go

Lines 41 to 43 in 6539513

func (v *Error) Error() string {

return fmt.Sprintf("%s: %s", v.Field, v.ErrorBody())

}

I'm really sorry to be so dense, but I've found that's often much more accurate to look at the actually rendered strings than mentally follow the string rendering code. Can you make a test that prints out the rendered error messages from the invalid cases and run it with and without your change here, so we can compare the difference? Or just manually put some invalid selectors through the API.

In a change like this I really want to be sure that the error messages are helping users more and not just making the code more regular.

It would like:

test yaml:

apiVersion: v1 kind: Pod metadata: name: with-node-affinity spec: affinity: nodeAffinity: requiredDuringSchedulingIgnoredDuringExecution: nodeSelectorTerms: - matchFields: - key: "metadata.name" operator: In values: - a - b - matchExpressions: - key: "invalid key" operator: In values: - b - c containers: - name: with-node-affinity image: k8s.gcr.io/pause:3.2

kube-scheduler has a similar output, just not pretty print like kubectl, because kubectl formats a http response, kube-scheduler simply call .Error().

This PR doesn't introduce this error format, it just makes labels.NewRequirement return all parsing errors.

Example (without/with this patch):

apiVersion: kubescheduler.config.k8s.io/v1beta1 kind: KubeSchedulerConfiguration profiles: - schedulerName: default-scheduler - schedulerName: test-err-scheduler pluginConfig: - name: NodeAffinity args: addedAffinity: requiredDuringSchedulingIgnoredDuringExecution: nodeSelectorTerms: - matchExpressions: - key: "invalid key" operator: Gt values: - b - c clientConnection: kubeconfig: kubeconfig

OK, thanks for humoring me :)

...I wouldn't say this is amazingly clear, but it's maybe slightly better than it was, and I think people will be able to figure it out.

lavalamp · 2021-01-11T22:16:06Z

I'd still like to see complete error messages as rendered for a human before signing off on this.

lavalamp · 2021-01-13T17:28:05Z

/lgtm
/approve

k8s-ci-robot · 2021-01-13T17:28:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, lavalamp, lingsamuel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/scheduler/OWNERS~~ [alculquicondor,lavalamp]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/component-helpers/scheduling/OWNERS~~ [alculquicondor,lavalamp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ref kubernetes/kubernetes#97538

* Upgrade to k/*@v0.21.1 in go.mod * [automated] make revendor for k/* dependencies This deletes pkg/mock/client-go/kubernetes/mocks.go to resolve the following deadlock: make revendor fails because of some dependencies of the file and make generate fails because of missing revendoring. File will be generated again in the next commit. * [automated] make generate for k/* dependencies * Upgrade to c-r@v0.9.0 in go.mod * [automated] make revendor for c-r dependency `make revendor` results in `hack/setup-envtest.sh` being broken, so reset the file after running `make revendor`. Adaption to breaking changes in the upstream file will be done in a later commit. * manager.NewClientBuilder was removed in favor of cluster.DefaultNewClient ref kubernetes-sigs/controller-runtime#1409 * client.*MergeFrom* now take client.Object instead of runtime.Object ref kubernetes-sigs/controller-runtime#1395 * [automated] make generate for c-r dependency * Adapt to changes in labels.NewRequirement ref kubernetes/kubernetes#97538 * Adapt to new setup-envtest tool Makes use of the new setup-envtest tool (kubernetes-sigs/controller-runtime#1488) in hack/setup-envtest.sh instead of vendoring hack/setup-envtest.sh and fetching binaries with that. * [automated] make revendor for setup-envtest tool * Adapt pkg/envtest to upstream changes - Make use of the new Users concept in envtest to provision a dedicated user for gardener-apiserver and a valid kubeconfig - Make use of the new way to configure API server args to easily configure kube-aggregator flags - Also generate certs for aggregation layer on our own instead of reusing the API server ca/certs (which is semantically correct), which allows us to drop our fork including kubernetes-sigs/controller-runtime#1449 * Styling nits

k8s-ci-robot requested review from caesarxuchao and liu-cong December 28, 2020 04:45

lingsamuel force-pushed the requirement-return-field-error branch 5 times, most recently from 9de087d to 2ee3655 Compare December 28, 2020 05:03

k8s-ci-robot requested a review from alculquicondor December 28, 2020 05:05

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 28, 2020

lingsamuel force-pushed the requirement-return-field-error branch 2 times, most recently from a98b154 to 0a3c5ca Compare December 28, 2020 07:20

chendave reviewed Dec 29, 2020

View reviewed changes

lingsamuel requested a review from chendave December 29, 2020 07:21

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Jan 7, 2021

lingsamuel force-pushed the requirement-return-field-error branch from 7095b21 to 9438c59 Compare January 7, 2021 13:45

alculquicondor reviewed Jan 7, 2021

View reviewed changes

staging/src/k8s.io/apimachinery/pkg/labels/selector.go Outdated Show resolved Hide resolved

staging/src/k8s.io/component-helpers/scheduling/corev1/nodeaffinity/nodeaffinity.go Outdated Show resolved Hide resolved

lingsamuel force-pushed the requirement-return-field-error branch from 9438c59 to 5e8f812 Compare January 8, 2021 08:36

lingsamuel requested a review from alculquicondor January 8, 2021 08:36

lingsamuel force-pushed the requirement-return-field-error branch from 5e8f812 to 96ed831 Compare January 8, 2021 10:55

alculquicondor reviewed Jan 8, 2021

View reviewed changes

make labels.NewRequirement returns aggregated field.ErrorList, make n…

a1f8dc4

…odeaffinity parsing function use it Signed-off-by: Ling Samuel <lingsamuelgrace@gmail.com>

lingsamuel force-pushed the requirement-return-field-error branch from 96ed831 to a1f8dc4 Compare January 11, 2021 08:24

lavalamp reviewed Jan 11, 2021

View reviewed changes

lingsamuel requested a review from lavalamp January 12, 2021 09:34

k8s-ci-robot assigned lavalamp Jan 13, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 13, 2021

k8s-ci-robot merged commit a28c802 into kubernetes:master Jan 13, 2021

k8s-ci-robot added this to the v1.21 milestone Jan 13, 2021

lingsamuel deleted the requirement-return-field-error branch January 25, 2021 13:40

timebertt added a commit to timebertt/gardener that referenced this pull request Jun 11, 2021

Adapt to changes in labels.NewRequirement

6a77de4

ref kubernetes/kubernetes#97538

timebertt added a commit to timebertt/gardener that referenced this pull request Jun 14, 2021

Adapt to changes in labels.NewRequirement

de4d9fd

ref kubernetes/kubernetes#97538

timebertt added a commit to timebertt/gardener that referenced this pull request Jun 15, 2021

Adapt to changes in labels.NewRequirement

86884c0

ref kubernetes/kubernetes#97538

timebertt added a commit to timebertt/gardener that referenced this pull request Jun 16, 2021

Adapt to changes in labels.NewRequirement

eb8f950

ref kubernetes/kubernetes#97538

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make labels.NewRequirement returns aggregated field.ErrorList #97538

make labels.NewRequirement returns aggregated field.ErrorList #97538

lingsamuel commented Dec 28, 2020 •

edited

k8s-ci-robot commented Dec 28, 2020

lingsamuel commented Dec 28, 2020

yuxiaobo96 commented Dec 28, 2020

alculquicondor left a comment

lingsamuel commented Jan 11, 2021

lingsamuel commented Jan 11, 2021

lavalamp Jan 11, 2021

lingsamuel Jan 12, 2021

lavalamp Jan 12, 2021

alculquicondor Jan 13, 2021

lavalamp Jan 11, 2021

lingsamuel Jan 12, 2021

lavalamp Jan 12, 2021

lingsamuel Jan 13, 2021

lingsamuel Jan 13, 2021

lingsamuel Jan 13, 2021

lavalamp Jan 13, 2021

lavalamp commented Jan 11, 2021

lavalamp commented Jan 13, 2021

k8s-ci-robot commented Jan 13, 2021

	if !qualifiedNameRegexp.MatchString(name) {
	errs = append(errs, "name part "+RegexError(qualifiedNameErrMsg, qualifiedNameFmt, "MyName", "my.name", "123-abc"))
	}

	func validateLabelKey(k string, path field.Path) field.Error {
	if errs := validation.IsQualifiedName(k); len(errs) != 0 {
	return field.Invalid(path, k, strings.Join(errs, "; "))

	func (v *Error) Error() string {
	return fmt.Sprintf("%s: %s", v.Field, v.ErrorBody())
	}

make labels.NewRequirement returns aggregated field.ErrorList #97538

make labels.NewRequirement returns aggregated field.ErrorList #97538

Conversation

lingsamuel commented Dec 28, 2020 • edited

k8s-ci-robot commented Dec 28, 2020

lingsamuel commented Dec 28, 2020

yuxiaobo96 commented Dec 28, 2020

alculquicondor left a comment

Choose a reason for hiding this comment

lingsamuel commented Jan 11, 2021

lingsamuel commented Jan 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Jan 11, 2021

lavalamp commented Jan 13, 2021

k8s-ci-robot commented Jan 13, 2021

lingsamuel commented Dec 28, 2020 •

edited